Agent decorator fix #976
Agent decorator fix #976
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@fenilfaldu I have merged changes related to linting, please refactor to pass unit test :) |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces async context manager support into the agent decorator to ensure proper span lifecycle cleanup during asynchronous operations. The key changes are:
- Addition of a new test in tests/unit/sdk/test_decorators.py to verify async context manager behavior and object deletion.
- Updates in agentops/sdk/decorators/factory.py to implement async aenter, async aexit, and enhanced del logic for robust span cleanup.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/sdk/test_decorators.py | Added tests for async context manager and del cleanup logic. |
| agentops/sdk/decorators/factory.py | Updated the decorator factory to support async context management. |
Comments suppressed due to low confidence (1)
agentops/sdk/decorators/factory.py:77
- [nitpick] Since aexit is implemented as an async method, verify that calling the synchronous exit method on the span context manager does not block the event loop. If the underlying span context manager supports async cleanup, consider using it instead.
self._agentops_span_context_manager.__exit__(exc_type, exc_val, exc_tb)
Dwij1704
left a comment
Dwij1704
left a comment
There was a problem hiding this comment.
we can safely ditch del here - the context managers in init and aexit have got our backs for both sync and async cleanup. It's like having two people doing the same job - might as well let the more reliable one handle it!
There was a problem hiding this comment.
Pull Request Overview
This PR adds async context manager support to class decorators for proper span lifecycle handling in asynchronous workflows and includes a new test to cover async enter/exit and destructor cleanup.
- Introduce
__aenter__and__aexit__inWrappedClassfor async context manager support. - Remove the existing
__del__method (unintentionally dropping sync cleanup). - Add a new async test
test_async_context_manager_and_del_coverageintests/unit/sdk/test_decorators.py.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/unit/sdk/test_decorators.py | Added gc, pytest imports and test_async_context_manager_and_del_coverage. |
| agentops/sdk/decorators/factory.py | Removed __del__; added __aenter__/__aexit__ methods to support async usage. |
Comments suppressed due to low confidence (3)
tests/unit/sdk/test_decorators.py:632
- [nitpick] After forcing garbage collection, add assertions to verify that the instance's span references (
_agentops_span_context_manager,_agentops_active_span) have been cleared to confirm proper cleanup.
gc.collect() # Force garbage collection to trigger __del__
tests/unit/sdk/test_decorators.py:2
- [nitpick] Group and sort standard library imports together (e.g.,
asyncio,gc) and separate them from third-party imports (pytest), following PEP8 import ordering.
import asyncio
agentops/sdk/decorators/factory.py:53
- The original
__del__destructor was removed, so spans started in__init__may never be closed for non-async usage. Reintroduce a__del__method that calls the context manager's__exit__to ensure proper cleanup in synchronous scenarios.
async def __aenter__(self):
There was a problem hiding this comment.
Pull Request Overview
This pull request adds async context manager support for span lifecycles in async operations to better ensure proper cleanup and prevent premature span exports. Key changes include:
- Adding async aenter/aexit methods to the decorated classes.
- Updating tests to cover async context management and cleanup logic.
- Removing the synchronous del cleanup in favor of async cleanup.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/sdk/test_decorators.py | Added tests for the new async context management support. |
| agentops/sdk/decorators/factory.py | Updated the decorator to support async lifecycle management by implementing aenter and aexit. |
2 participants
📥 Pull Request
📘 Description
Added async context manager support to handle span lifecycles in async operations, ensures proper cleanup and preventing premature span exports.
🧪 Testing
Verified span lifecycle management in async contexts and confirmed backward compatibility with existing sync initialization.